Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable executing atm_bdy_set_scalars_work on gpus #1265

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Enable executing atm_bdy_set_scalars_work on gpus #1265

wants to merge 1 commit into from
+13 −0

Conversation

jim-p-w
Copy link
Contributor

@jim-p-w jim-p-w commented Jan 16, 2025 •
edited
Loading

This PR enables executing the atm_bdy_set_scalars_work subroutine on GPUs.
This is accomplished using OpenACC directives.

Tested with a regional test case.
Baseline results were obtained from building the develop branch with:
make -j4 nvhpc CORE=atmosphere PRECISION=single OPENACC=true
Then the changes in this PR were made and compiled in the same way.

Comparing the results stored in the restart.*.nc file showed no changes.

@jim-p-w jim-p-w marked this pull request as draft January 16, 2025 19:56
@jim-p-w jim-p-w force-pushed the atmosphere/acc_atm_bdy_set_scalars_work branch from a7d955d to fad0fb1 Compare January 16, 2025 22:56
@jim-p-w jim-p-w changed the title Add acc directives to enable executing atm_bdy_set_scalars_work on gpus Enable executing atm_bdy_set_scalars_work on gpus Jan 16, 2025
@jim-p-w jim-p-w marked this pull request as ready for review January 16, 2025 23:08
@mgduda mgduda added Atmosphere OpenACC Work related to OpenACC acceleration of code labels Jan 17, 2025
@jim-p-w jim-p-w force-pushed the atmosphere/acc_atm_bdy_set_scalars_work branch from fad0fb1 to 982098f Compare January 17, 2025 22:15
@jim-p-w jim-p-w force-pushed the atmosphere/acc_atm_bdy_set_scalars_work branch from 982098f to 7d1d937 Compare January 18, 2025 00:04
@abishekg7
Copy link
Collaborator

Wondering if we also need a timer start/stop around the call to atm_bdy_set_scalars_work. Other than that everything looks good!

@gdicker1
Copy link
Collaborator

Wondering if we also need a timer start/stop around the call to atm_bdy_set_scalars_work. Other than that everything looks good!

I'm wondering the same, but that would probably be another PR. @mgduda what are your thoughts about these timers? Maybe we don't need parent timers for the _bdy_ routines since theses [ACCxfer] timers are meant to be transient?

@jim-p-w jim-p-w force-pushed the atmosphere/acc_atm_bdy_set_scalars_work branch from 7d1d937 to ec21faf Compare January 18, 2025 00:26
@mgduda
Copy link
Contributor

mgduda commented Jan 18, 2025

The [ACC_data_xfer] timers are intended to be transient, and I expect we'll be removing them when we remove the data transfers for individual routines. If the atm_bdy_set_scalars_work routine doesn't already have a timer, then I suppose the atm_bdy_set_scalars_work [ACC_data_xfer] has less value. Perhaps for now, we can just include the [ACC_data_xfer] timer (especially since it's already been added).

@jim-p-w The code changes themselves look fine to me. Can you modify the commit message:

  1. "acc" -> "OpenACC"
  2. "gpus" -> "GPUs"
  3. note that the commit includes a "atm_bdy_set_scalars_work [ACC_data_xfer]" timer although there is no corresponding "atm_bdy_set_scalars_work" timer that includes the computational work of the routine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abishekg7 abishekg7 abishekg7 left review comments

@gdicker1 gdicker1 gdicker1 requested changes

@mgduda mgduda mgduda requested changes

Assignees
No one assigned
Labels
Atmosphere OpenACC Work related to OpenACC acceleration of code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jim-p-w @abishekg7 @gdicker1 @mgduda